Skip to content

Additional tce tests #144650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Additional tce tests #144650

wants to merge 1 commit into from
+64 −2

Conversation

Borgerr
Copy link
Contributor

@Borgerr Borgerr commented Jul 29, 2025 •
edited by rustbot
Loading

r? @oli-obk

Adds known-bug tests for LLVM emissions regarding indirect operands for TCE. Also includes a test, indexer.rs, referring to function_table behavior described by the RFC.

Depends on #144232

Closes #144293

@rustbot
Copy link
Collaborator

rustbot commented Jul 29, 2025

oli-obk is not on the review rotation at the moment.
They may take a while to respond.

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 29, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 29, 2025

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rustbot

This comment has been minimized.

Sign in to view
@Borgerr Borgerr force-pushed the additional-tce-tests branch from cdfbab1 to 06db3a5 Compare July 29, 2025 20:02
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Borgerr Borgerr force-pushed the additional-tce-tests branch from 06db3a5 to 0dba4bb Compare July 29, 2025 20:52
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor

oli-obk commented Jul 30, 2025

You're gonna need the kind of //@normalize-stderr attributes as other ICE tests have, or just add the test to tests/crashes

@Borgerr Borgerr force-pushed the additional-tce-tests branch from 0dba4bb to e0738ad Compare July 30, 2025 14:49
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Borgerr Borgerr force-pushed the additional-tce-tests branch from e0738ad to e0b4595 Compare July 30, 2025 16:55
WaffleLapkin
WaffleLapkin reviewed Jul 30, 2025
View reviewed changes
tests/ui/explicit-tail-calls/indexer.rs Outdated
Comment on lines 1 to 3
// Indexing taken from
// https://github.com/phi-go/rfcs/blob/guaranteed-tco/text%2F0000-explicit-tail-calls.md#tail-call-elimination
// should probably come back to after some decision on verbiage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "decision on verbiage"?.. What is this test even testing?

you can make the test pass (hopefully) by just replacing &dyn Fn(usize) with fn(usize)

Copy link
Contributor Author

@Borgerr Borgerr Aug 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Decision on verbiage" was meant to indicate more discussion on examples given in that part of the RFC. From what I could see, nobody had tested that particular example of tail-calling something from a function table, and I was curious as to whether it'd work as desired, as it's kind of an operation. Didn't look too close in that regard though, admittedly.
I can indicate that better with a new comment, thanks for pointing that out.

Test also does pass with the described change. Push coming in a moment. However, it does feel a little silly to include now, so if input indicates I should remove it, I will. I suppose another approach that would include indexing would be something like a "basic examples from RFC" sort of file, or just appending this behavior to another test.

@Borgerr Borgerr force-pushed the additional-tce-tests branch from e0b4595 to 1f4a88a Compare August 2, 2025 16:31
WaffleLapkin
WaffleLapkin reviewed Aug 2, 2025
View reviewed changes
@Borgerr Borgerr force-pushed the additional-tce-tests branch from 1f4a88a to c9898dc Compare August 3, 2025 00:17
@WaffleLapkin
Copy link
Member

@Borgerr I think you broke something in rebase, since you have a commit which is not yours...

@WaffleLapkin WaffleLapkin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 3, 2025
@Borgerr Borgerr force-pushed the additional-tce-tests branch from c9898dc to caa3cf1 Compare August 3, 2025 14:50
@Borgerr
Copy link
Contributor Author

Borgerr commented Aug 3, 2025

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 3, 2025
@WaffleLapkin
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 3, 2025

📌 Commit caa3cf1 has been approved by WaffleLapkin

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2025
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 6, 2025
tgross35
tgross35 reviewed Aug 6, 2025
View reviewed changes
@Borgerr Borgerr force-pushed the additional-tce-tests branch from 1fe1bf5 to 27797b1 Compare August 6, 2025 19:15
@rustbot
Copy link
Collaborator

rustbot commented Aug 6, 2025

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

@Borgerr
Copy link
Contributor Author

Borgerr commented Aug 6, 2025

@rustbot label -S-waiting-on-author +S-waiting-on-review

Just moving this to tests/crashes since this is getting a tad silly. Hope this works 🙏

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2025
@Borgerr
Copy link
Contributor Author

Borgerr commented Aug 6, 2025

@rustbot label +S-waiting-on-author -S-waiting-on-review

Wrong issue in file name

@tgross35
Copy link
Contributor

tgross35 commented Aug 6, 2025

Please give the test a descriptive name, we're trying to move away from test files that only have a number. We can run the try though

@bors2 try jobs=dist-i586-gnu-i586-i686-musl

@rustbot label +S-waiting-on-author -S-waiting-on-review

For future reference, @rustbot author or @rustbot review are aliases to update both labels

@rust-bors
Copy link

rust-bors bot commented Aug 6, 2025

⌛ Trying commit 27797b1 with merge aab23ff…

To cancel the try build, run the command @bors try cancel.

rust-bors bot added a commit that referenced this pull request Aug 6, 2025
@rust-bors
Auto merge of #144650 - Borgerr:additional-tce-tests, r=<try>
aab23ff 
Additional tce tests

try-job: dist-i586-gnu-i586-i686-musl
@rust-bors
Copy link

rust-bors bot commented Aug 6, 2025

☀️ Try build successful (CI)
Build commit: aab23ff (aab23ffa93515b947f652645af544d32d168a8e4, parent: 29cdc6a109ee98a382f974bf89d3971b4385399c)

@WaffleLapkin
Copy link
Member

r? tgross35

@rustbot rustbot assigned tgross35 and unassigned oli-obk Aug 6, 2025
@Borgerr
explicit tail call tests with indirect operands in LLVM, small test f…
916fb6a 
…or indexing into a function table as described by RFC 3407
@Borgerr Borgerr force-pushed the additional-tce-tests branch from 27797b1 to 916fb6a Compare August 7, 2025 00:14
@rustbot rustbot added the F-explicit_tail_calls `#![feature(explicit_tail_calls)]` label Aug 7, 2025
@Borgerr
Copy link
Contributor Author

Borgerr commented Aug 7, 2025

@rustbot ready

Thank you guys again for your patience on this one, this should be it 🙏

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 7, 2025
@Borgerr Borgerr requested a review from tgross35 August 7, 2025 00:15
@tgross35
Copy link
Contributor

tgross35 commented Aug 7, 2025

@bors r=WaffleLapkin,tgross35

@bors
Copy link
Collaborator

bors commented Aug 7, 2025

📌 Commit 916fb6a has been approved by WaffleLapkin,tgross35

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 7, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Aug 7, 2025
@Zalathar
Rollup merge of rust-lang#144650 - Borgerr:additional-tce-tests, r=Wa…
3636042 
…ffleLapkin,tgross35

Additional tce tests

r? `@oli-obk`

Adds known-bug tests for LLVM emissions regarding indirect operands for TCE. Also includes a test, `indexer.rs`, referring to function_table behavior described by the RFC.

Depends on rust-lang#144232

Closes rust-lang#144293
@Zalathar Zalathar mentioned this pull request Aug 7, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WaffleLapkin WaffleLapkin WaffleLapkin approved these changes

@tgross35 tgross35 Awaiting requested review from tgross35

Assignees

@tgross35 tgross35

Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. F-explicit_tail_calls `#![feature(explicit_tail_calls)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tail calls with indirect operands are untested
8 participants
@Borgerr @rustbot @rust-log-analyzer @oli-obk @WaffleLapkin @bors @samueltardieu @tgross35